-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Fast path for NodeEventGenerator #17019
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
function tryParseSelector(rawSelector, knownTypes) { | ||
|
||
const rawSelectorWithoutExit = rawSelector.replace(/:exit$/u, ""); | ||
|
||
// no need to parse known types | ||
if (knownTypes.has(rawSelectorWithoutExit)) { | ||
return { | ||
type: "identifier", | ||
value: rawSelectorWithoutExit | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the ESLint configuration for our codebase (enabled rules have 195 different selectors, out of which 113 are identifiers), this change improves performance by a couple of ms. I'm getting different results for the same run, but it's like ~16ms instead of ~18ms in total spent for running this function. Since we are caching parsed selectors, this improvement doesn't scale with the number of files being linted, so it looks like it won't bring much.
I'm going to tweet this out to see if anyone is willing to try this branch and post their results. It's possible we aren't seeing the change but others might. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Don't close. Still this solution needs to be experimented. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Since there is a small speedup in our own tests, maybe it's worth merging this? I wonder if it might help others in the wild more than us. |
Thanks for working on this @nzakas! If you can show the method(s) used to measure execution time before and after the change, I'd like to try this on my machine and see the results I'm getting. |
I just ran a combination of |
Using VSCode profiler to time function execution in Before
After
Note that the The differences are really minimal though, and there is a large margin for errors. |
@@ -260,9 +274,10 @@ class NodeEventGenerator { | |||
this.exitSelectorsByNodeType = new Map(); | |||
this.anyTypeEnterSelectors = []; | |||
this.anyTypeExitSelectors = []; | |||
this.knownTypes = new Set(Object.keys(esqueryOptions.visitorKeys)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if it's really necessary to keep track of known node types when the goal is to detect simple selectors. Maybe we could just test the selector string (in tryParseSelector
and applySelector
) for special characters like >
, +
, [
, ]
, :
, ,
, *
, !
, ~
white spaces, etc. A valid selector is a simple selector if it does not contain any special characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, but I think doing such a check could be slower than keeping track of known Node types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also not sure, so I decided to test it (spoiler: it didn't help)
I defined a regex
const SIMPLE_SELECTOR_REGEX = /^\w+$/;
It's not perfect, but it matches all standard ESTree node names plus the code path analysis events (which are intended to work like identifiers as it seems). Then I replaced this.knownTypes.has()
with SIMPLE_SELECTOR_REGEX.test()
here and here and cleared the remaining references to knownTypes
.
I run the VSCode profiler on npm run lint
to measure execution time and compared the results with knownTypes
and with my regex. No difference though. The times are almost the same either way. I guess we can keep the knownTypes
and move on.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
@fasttime yeah, running on our own repo doesn't show any significant improvement, but there have been previous changes that didn't show a big improvement on our repo and yet did show a big improvement in someone else's. The way ESLint is configured makes a big difference so the only way we can really know is to try the change out on a few other repos that perhaps have more complicated configurations. |
this.emitter.emit(selector.rawSelector, node); | ||
return; | ||
} | ||
|
||
if (esquery.matches(node, selector.parsedSelector, this.currentAncestry, this.esqueryOptions)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we merge this condition with the next one in order to avoid repeating this.emitter.emit(selector.rawSelector, node);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think you meant to post this on line 312?
I prefer the way I have it written because it's easier to see that there are two distinct conditions being handled. I don't mind repeating that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think you meant to post this on line 312?
Yes, I should have clicked on that line rather than the whole changed block, sorry.
I prefer the way I have it written because it's easier to see that there are two distinct conditions being handled. I don't mind repeating that line.
Fair enough. That would also not affect performance, only style.
Thanks! The logic LGTM, but somehow I don't see a significant improvement here, not even in |
lib/linter/node-event-generator.js
Outdated
if (this.knownTypes.has(selector.rawSelector)) { | ||
this.emitter.emit(selector.rawSelector, node); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a property isIdentifier: parsedSelector.type === "identifier"
to selectors and just check if (selector.isIdentifier)
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I’m missing some context here. Why would we do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For performance reasons. Whether the selector is an identifier is already known at the parse time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, are you saying that instead of checking knownTypes
we could just check to see if the selector is an identifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at this point if it's an identifier then it certainly matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Updated.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Not stale. @nzakas there's a review for you to look into. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic LGTM, but I'm consistently getting a worse performance after this change when running npm run test:performance
and time node bin/eslint .
, so I'm not sure whether we should merge this. I checked the blog post again but couldn't find performance tests for this recommendation.
Thanks for the update. Let's just close this then, it's not important. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Perf changes
What changes did you make? (Give an overview)
Added a "fast path" for NodeEventGenerator that skips matching and parsing of selectors when they are simple.
Refs #16962
Is there anything you'd like reviewers to focus on?
I'm not seeing any noticeable improvement with this work. Did I do something wrong?